Skip to content

Make bootloader honour the MCU Security Bit #586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Dec 24, 2020
Merged

Make bootloader honour the MCU Security Bit #586

merged 4 commits into from
Dec 24, 2020

Conversation

ksmith3036
Copy link
Contributor

If Security Bit is set, the MCU denies access to most information when conneting using an SWD-connected debugger. One has to perform a Chip Erase to access the MCU.

The default Arduino MKR bootloaders on the other hand, does not honour the Security Bit, and happily lets the bossac.exe program read its flash memory. This might have security or IP implications.

This pull request is a proposed fix for issue #570.

The proposed changes to the source code will not let a client application read from flash memory, and will always erase the full sketch flash memory, when flashing, to avoid anyone trying to perform a partial flash.

If BOOTPROT is set to 2 and Security Bit is set, the bootloader will be fully protected, and it should also not be trivial to read the sketch out of the MCU.
Sketches may of course still be flashed into the board.

Protection may also be turned on by setting the SECURE_BY_DEFAULT compile flag. This will not protect against flashing the bootloader itself, but will read-protect the flash storage.

To allow the MKR VIDOR 4000 loader to build to less than 8 KByte, the string handling of compile time DATE and TIME had to be concatinated in source code.

@facchinm
Copy link
Member

Hi @ksmith3036 ,
thank you very much for posting this!
I think the rationale is perfect but I have some "style" suggestions.

  1. Try keeping the original spaces/tabs formatting so the diff improves significanly (and also the ability to properly review)
  2. Use curly braces on single line ifs (even if C allows this the result is very prone to misread/errors)
  3. Split the patch into 3 or more atomic diffs (one could be date/time, the second b_security_enabled related bits, the third adding SECURE_BY_DEFAULT
    Apart from that, well done! I'll be superhappy to merge this once it's fixed!

@ksmith3036
Copy link
Contributor Author

  1. Try keeping the original spaces/tabs formatting so the diff improves significanly (and also the ability to properly review)

OK, will do.

  1. Use curly braces on single line ifs (even if C allows this the result is very prone to misread/errors)

OK, will do, even if it hurts to split things that syntactically belongs together. :-)

  1. Split the patch into 3 or more atomic diffs (one could be date/time, the second b_security_enabled related bits, the third adding SECURE_BY_DEFAULT

Here I am a bit unsure, because without concatening the DATE and TIME,s the Vidor Bootloader become just over 8 KByte in size.
But, I will comply.
But I certainly have a problem, because I have no clue about how to do it in Git. Do I revert my changes in some way and then commit each separately. I noticed that on another Git project, when I created a pull request, all my later commits automatically was attached to the same pull request.
Do the three commits need to be in three different pull requests?

@facchinm
Copy link
Member

Thank you so much!
So, in git you can do this (from your situation):

git reset HEAD~1 # this will "delete" your last commit but all the modification will still be there
git add -i # will launch an interactive shell where you can add pieces of code
# when done, just quit and commit as usual, for every "subcommit"
git push $yourremote master --force # --force rewrites history and keeps this PR up to date (without opening another one)

If you feel unsure I can recreate the patch myself, push in your repo for double checking and then we can merge 🙂

@ksmith3036
Copy link
Contributor Author

Thank you very much for the tips!
I will try myself, I really need to learn to use Git! :-)

@ksmith3036
Make bootloaders check for the MCU Security Bit.
7d69dd6 
If Security Bit is set, it will not let a client read from flash memory, and will always erase the full sketch flash memory, when flashing, to avoid anyone trying to perform a partial flash.

If BOOTPROT is set to 2 and Security Bit is set, the bootloader will be fully protected, and it should not be trivial to read the sketch out of the MCU.

Without these changes, the bootloader ignores the Security Bit, and let clients like bossac.exe read the entire flash memory.
@ksmith3036
Bootloader protection of flash memory may also be turned on by settin…
9cb10ad 
…g the SECURE_BY_DEFAULT compile flag, either through an argument to make, or by setting flag in sam_ba_monitor.h.
@ksmith3036
To allow the MKR VIDOR 4000 loader to build to less than 8 KByte, the…
efcc5c9 
… string handling of compile time DATE and TIME had to be concatinated in source code.

The side effect is that the code is much more concise and readable.
@ksmith3036
Sometimes an Arduino sketch needs to reset the MCU, but still wants t…
aafa688 
…o have some variables to be able to survive.

The bootloader itself uses very little RAM and excludes by default the last 4 bytes of RAM from use by the stack.
To allow sketches using a modified linker script to take the same approach, the changed bootloader linker script excludes the last 1 KByte of RAM from stack.
@ksmith3036
Copy link
Contributor Author

ksmith3036 commented Dec 23, 2020 •
edited
Loading

Thank you for your advice.
I split into three commits, hopefully as you want it.
I added one more commit, as I had forgotten it earlier on. My sketch needs to store a few variables able to survive a reset of the MCU, so I have modifed the Arduino linker script to have a "noinit" section of 256 bytes of RAM at the end of RAM, but this only works if the bootloader also keeps the stack out of the same area. Because of this, I modifed the bootloader linker script to exclude the last 1 KByte of RAM from use. The bootloader seems happy with this, and really should, having 31 KByte of RAM available! :-)
I hope that also could make it into the official bootloader.

@facchinm
Copy link
Member

That's perfect! Lovely PR!
I'm merging it now. About distribution, we usally follow this strategy:

  • on the next production batch, we'll rebuild the bootloaders and flash them on the boards.
  • once the boards have been manufactured / flashed we'll merge the compiled binaries so running "Burn Bootloader" from the IDE will update to something properly validated.

Thank you again!

@facchinm facchinm merged commit f35f157 into arduino:master Dec 24, 2020
@henrygab henrygab mentioned this pull request Apr 1, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ksmith3036 @facchinm